Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[core] GCS get all actors info no longer need to specify if dead jobs' actors need to be included #34415

Closed
wants to merge 3 commits into from
Closed

[core] GCS get all actors info no longer need to specify if dead jobs' actors need to be included #34415

wants to merge 3 commits into from

Conversation

rickyyx
Copy link
Contributor

@rickyyx rickyyx commented Apr 14, 2023 •
edited
Loading

Why are these changes needed?

With #31019, we always have up to 10k dead actors cached by default.

There's no existing usecase where we don't want to have actors from dead jobs (AFAIK)

With #34348, one could filter actors with job id as well.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@rickyyx
init
8377d82 
Signed-off-by: rickyyx <[email protected]>
@rickyyx rickyyx mentioned this pull request Apr 14, 2023
Merged
8 tasks
@rickyyx rickyyx marked this pull request as ready for review April 15, 2023 00:21
@rickyyx rickyyx added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Apr 15, 2023
rkooo567
rkooo567 approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @alanwguo is this flag used from you guys side?

@alanwguo
Copy link
Contributor

cc @alanwguo is this flag used from you guys side?

No, we don't use that flag today. In the progress bar summary API we currently get the actor name from the actor task (parse the actor name from <ActorName>.<method_name>). This was a hack because the actors list api did not return actors for dead jobs so we couldn't get the actor name from the actor id.

Now, we can use the actor api to get the actor name from actor ID but we haven't made that change yet.

@rickyyx rickyyx mentioned this pull request Apr 17, 2023
Open
@rickyyx
conflcit
75a61cc 
Signed-off-by: rickyyx <[email protected]>
@wuisawesome
Copy link
Contributor

Don't have a strong opinion here. Just for historical context, IIRC the 10k actor limit and per-job cleanup were there to avoid leaking memory.

@rkooo567
Copy link
Contributor

cc @rickyyx do you think it is hard to handle @alanwguo's comment in this PR? Seems like an easy change.

@rkooo567
Copy link
Contributor

cc @scv119 to review gcs_service proto changes

@rickyyx rickyyx added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Apr 18, 2023
@stale
Copy link

stale bot commented Jun 10, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 10, 2023
@rickyyx
Copy link
Contributor Author

rickyyx commented Jun 12, 2023

TODO

@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jun 12, 2023
@stale
Copy link

stale bot commented Jul 15, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jul 15, 2023
@rkooo567 rkooo567 removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jul 18, 2023
@stale
Copy link

stale bot commented Sep 17, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Sep 17, 2023
@stale
Copy link

stale bot commented Oct 15, 2023

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!

@stale stale bot closed this Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkooo567 rkooo567 rkooo567 approved these changes

@wuisawesome wuisawesome Awaiting requested review from wuisawesome

@ijrsvt ijrsvt Awaiting requested review from ijrsvt

@edoakes edoakes Awaiting requested review from edoakes

@alanwguo alanwguo Awaiting requested review from alanwguo

@architkulkarni architkulkarni Awaiting requested review from architkulkarni

@ericl ericl Awaiting requested review from ericl

@AmeerHajAli AmeerHajAli Awaiting requested review from AmeerHajAli

@robertnishihara robertnishihara Awaiting requested review from robertnishihara

@pcmoritz pcmoritz Awaiting requested review from pcmoritz pcmoritz is a code owner

@raulchen raulchen Awaiting requested review from raulchen raulchen is a code owner

@fishbone fishbone Awaiting requested review from fishbone

@scv119 scv119 Awaiting requested review from scv119

Assignees

@scv119 scv119

@wuisawesome wuisawesome

@rkooo567 rkooo567

Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. stale The issue is stale. It will be closed within 7 days unless there are further conversation tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rickyyx @alanwguo @wuisawesome @rkooo567 @scv119